The Securty Group name must be unique #53714
Conversation
Currently the service's name is not unique, and the Securty Group name is not unique too. openstack cloud provider will delete the Securty Group of other loadbalancer service when do a deletion.
k8s-ci-robot
added
release-note-none
|
/assign @dims |
FengyunPan
changed the title
|
/test pull-kubernetes-kubemark-e2e-gce |
|
/approve no-issue |
k8s-ci-robot
added
the
lgtm
|
/sig openstack |
k8s-ci-robot
added
the
area/provider/openstack
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, FengyunPan Associated issue requirement bypassed by: dims The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test pull-kubernetes-e2e-gce-gpu |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
There was a problem hiding this comment.
Well spotted!
How do we make this work for upgrades? I think we need to look up the old name during update/delete (not create) if the new name doesn't exist - at least for a release (add a dated deprecation comment so we remember when it's safe to remove). It's probably going to be easiest if we just forcibly update the name when we find the old name (assuming we can do that?), to ensure the transition happens within our deprecation window.
| @@ -372,7 +372,7 @@ func popMember(members []v2pools.Member, addr string, port int) []v2pools.Member | |||
| } | |||
|
|
|||
| func getSecurityGroupName(clusterName string, service *v1.Service) string { | |||
| return fmt.Sprintf("lb-sg-%s-%v", clusterName, service.Name) | |||
| return fmt.Sprintf("lb-sg-%s-%s-%s", clusterName, service.Namespace, service.Name) | |||
There was a problem hiding this comment.
Another alternative would be to use the service UID, which is also unique over time (but less readable).
If I delete a service and then quickly create a new one with the same namespace/name - what do we want to have happen to the securityGroup?
I think we want the securityGroup to not overlap in this case (so the old one can be cleaned up in parallel with the new one being created, without conflicts).
... So I think this means that this function should return
fmt.Sprintf("lb-sg-%s", service.UID)There was a problem hiding this comment.
If I delete a service and then quickly create a new one with the same namespace/name - what do we want to have happen to the securityGroup?
That make sense. The service.UID is less readable.
How about "fmt.Sprintf("lb-sg-%s-%s-%s", clusterName, service.Namespace, service.Name, service.UID)"?
There was a problem hiding this comment.
That's now very long - is there any length limit that we need to worry about here? A quick look at the code seems to imply that the name is limited to 255 chars, so I think we're ok on length.
Another (better?) option would be to use the securityGroup "description" field (also 255 chars) rather than trying to mash all our user-friendly text in the name field. That way we can have spaces, etc and change the specific text over time without worrying about wider impact.
Personally, I think the user is going to have a pretty good idea of which cluster a securityGroup is related to, and will be able to quickly find the relevant Service based on context, ports referred to, etc without needing any additional help. I agree that the UID is less readable though - so I agree that either your long-name version or the above name+description version is better than my original short-name-only version.
There was a problem hiding this comment.
That's right, I will check the size of its name.
Automatic merge from submit-queue (batch tested with PRs 56520, 53764). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add service.UID into security group name Related to: #53714 **Release note**: ```release-note NONE ```
…Name Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. The Securty Group name must be unique Currently the service's name is not unique, and the Securty Group name is not unique too. openstack cloud provider will delete the Securty Group of other loadbalancer service when do a deletion. OpenStack cloud provider get the ID of Securty Group by name, so the Securty Group name must be unique. https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go#L1262 **Release note**: ```release-note NONE ```
Automatic merge from submit-queue (batch tested with PRs 56520, 53764). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add service.UID into security group name Related to: kubernetes#53714 **Release note**: ```release-note NONE ```
Currently the service's name is not unique, and the Securty Group
name is not unique too. openstack cloud provider will delete the
Securty Group of other loadbalancer service when do a deletion.
OpenStack cloud provider get the ID of Securty Group by name, so the Securty Group name must be unique.
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go#L1262
Release note: